feat(davinci-client): support single checkbox component#629
Open
ancheetah wants to merge 1 commit into
Open
Conversation
|
📝 WalkthroughWalkthroughAdds SingleCheckboxField and ValidatedBooleanCollector across types, collector factory, Redux reducer, client update typing, API reports, and an e2e checkbox component; also updates tsconfig references and .gitignore. ChangesValidatedBooleanCollector Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
View your CI Pipeline Execution ↗ for commit af9e242 ☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
e2e/davinci-app/components/boolean.ts (2)
1-6: 💤 Low valueNitpick: Copyright year inconsistency.
This file uses copyright year 2026, while other files in the codebase use 2025. Consider using 2025 for consistency, unless this is intentional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/davinci-app/components/boolean.ts` around lines 1 - 6, Update the copyright header year from 2026 to 2025 in the top-of-file comment block in e2e/davinci-app/components/boolean.ts so it matches the rest of the repository; edit the existing comment lines (the block that starts with "Copyright (c) 2026 Ping Identity Corporation") to use 2025 instead.
24-28: 💤 Low valueOptional: Duplicate label might cause confusion.
The component creates a group label (lines 26-28) and an individual checkbox label (lines 41-43), both displaying the same text from
collector.output.label. For a single checkbox component, this creates visual duplication. Consider removing the group label or differentiating the text.♻️ Possible refinement
- // Create a heading/label for the checkbox group - const groupLabel = document.createElement('div'); - groupLabel.textContent = collector.output.label || 'Single Checkbox'; - groupLabel.className = 'single-checkbox-label'; - containerDiv.appendChild(groupLabel); - // Create checkboxes for each option const wrapper = document.createElement('div'); wrapper.className = 'checkbox-wrapper';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/davinci-app/components/boolean.ts` around lines 24 - 28, The component currently creates a duplicate visible label by appending a groupLabel (const groupLabel) with text from collector.output.label and also rendering an individual checkbox label later, causing visual duplication in the single-checkbox UI; fix by removing or hiding the redundant group label when rendering a single checkbox — either stop creating/appending groupLabel (remove the containerDiv.appendChild(groupLabel) lines) or change groupLabel.textContent to a different contextual string (or add a CSS class to visually hide it) and ensure the individual checkbox label (the per-checkbox label element) remains the primary visible label.packages/davinci-client/src/lib/collector.types.ts (1)
634-635: 💤 Low valueFormatting change unrelated to PR objective.
The reformatting of
AttestationValueandAssertionValuefrom multiline to single-lineOmitexpressions appears unrelated to the checkbox support feature. While this doesn't change functionality, consider whether this formatting change belongs in a separate PR focused on code style.Also applies to: 653-654
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/davinci-client/src/lib/collector.types.ts` around lines 634 - 635, The PR contains unrelated formatting changes: the multiline Omit types for AttestationValue and AssertionValue were collapsed into single-line forms; revert these formatting-only edits so they remain multiline (as before) and move any styling-only changes to a separate PR. Locate the AttestationValue and AssertionValue type declarations in collector.types.ts and restore their original multiline formatting (preserving the exact type members: Omit<PublicKeyCredential, 'rawId' | 'response' | 'getClientExtensionResults' | 'toJSON'> and Omit<PublicKeyCredential, 'rawId' | 'response' | 'getClientExtensionResults' | 'toJSON'> respectively), ensuring no functional changes are introduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/davinci-app/components/boolean.ts`:
- Around line 34-39: The SingleCheckboxField's required flag isn't applied to
the created checkbox element; update the element creation in the component that
builds the checkbox (where checkbox is created from collector.output) to set
checkbox.required = !!collector.output.required (and optionally
checkbox.setAttribute('aria-required','true') for accessibility) so the HTML
required validation is honored; ensure you reference collector.output.required
and the checkbox element in the SingleCheckboxField handling logic.
- Around line 46-49: The change handler for the checkbox uses the static value
attribute and compares target.value === 'checked', which is always true; update
the event listener on checkbox (the callback passed to
checkbox.addEventListener('change', ...)) to call updater with the actual
boolean state using target.checked instead of comparing target.value, i.e.,
replace the expression target.value === 'checked' with target.checked so the
updater receives the real checked state.
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Line 250: The new AND check causes required-only text fields to skip
validation; revert the outer conditional in the text-field branch to use OR
(i.e. change "else if ('validation' in field && field.validation && 'regex' in
field.validation)" back to "else if ('validation' in field || 'required' in
field)") so that fields with either validation or required:true are handled by
the ValidatedTextCollector/validation logic (the inner branches already
distinguish regex vs required), preventing them from falling through to
SingleValueCollector.
In `@packages/davinci-client/src/lib/node.reducer.ts`:
- Around line 196-198: The SINGLE_CHECKBOX branch in node.reducer.ts currently
calls returnValidatedBooleanCollector(field, idx) without passing the prefill
value; update the case for 'SINGLE_CHECKBOX' to extract the data variable from
formData (same way other branches do) and call
returnValidatedBooleanCollector(field, idx, data). Then update
returnValidatedBooleanCollector to accept the extra data parameter and forward
it into returnSingleValueCollector so SINGLE_CHECKBOX supports prefilled values,
mirroring how returnTextCollector and returnSingleSelectCollector handle the
data.
---
Nitpick comments:
In `@e2e/davinci-app/components/boolean.ts`:
- Around line 1-6: Update the copyright header year from 2026 to 2025 in the
top-of-file comment block in e2e/davinci-app/components/boolean.ts so it matches
the rest of the repository; edit the existing comment lines (the block that
starts with "Copyright (c) 2026 Ping Identity Corporation") to use 2025 instead.
- Around line 24-28: The component currently creates a duplicate visible label
by appending a groupLabel (const groupLabel) with text from
collector.output.label and also rendering an individual checkbox label later,
causing visual duplication in the single-checkbox UI; fix by removing or hiding
the redundant group label when rendering a single checkbox — either stop
creating/appending groupLabel (remove the containerDiv.appendChild(groupLabel)
lines) or change groupLabel.textContent to a different contextual string (or add
a CSS class to visually hide it) and ensure the individual checkbox label (the
per-checkbox label element) remains the primary visible label.
In `@packages/davinci-client/src/lib/collector.types.ts`:
- Around line 634-635: The PR contains unrelated formatting changes: the
multiline Omit types for AttestationValue and AssertionValue were collapsed into
single-line forms; revert these formatting-only edits so they remain multiline
(as before) and move any styling-only changes to a separate PR. Locate the
AttestationValue and AssertionValue type declarations in collector.types.ts and
restore their original multiline formatting (preserving the exact type members:
Omit<PublicKeyCredential, 'rawId' | 'response' | 'getClientExtensionResults' |
'toJSON'> and Omit<PublicKeyCredential, 'rawId' | 'response' |
'getClientExtensionResults' | 'toJSON'> respectively), ensuring no functional
changes are introduced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 524a05e9-77c1-451f-9f1a-fa7ce5b9287b
📒 Files selected for processing (12)
.gitignoree2e/davinci-app/components/boolean.tse2e/davinci-app/main.tspackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/client.types.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.types.test-d.tspackages/davinci-client/src/lib/node.types.tspackages/davinci-client/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/davinci-client/tsconfig.json
Comment on lines
+34
to
+39
| const checkbox = document.createElement('input'); | ||
| checkbox.type = 'checkbox'; | ||
| checkbox.id = collector.output.key; | ||
| checkbox.name = collector.output.key || 'single-checkbox-field'; | ||
| checkbox.checked = collector.output.value as boolean; | ||
| checkbox.value = 'checked'; |
There was a problem hiding this comment.
Minor: Consider adding required attribute support.
The SingleCheckboxField type includes a required property, but the component doesn't set the HTML required attribute on the checkbox element. This means required validation won't work as expected.
🛡️ Proposed fix
const checkbox = document.createElement('input');
checkbox.type = 'checkbox';
checkbox.id = collector.output.key;
checkbox.name = collector.output.key || 'single-checkbox-field';
checkbox.checked = collector.output.value as boolean;
checkbox.value = 'checked';
+ if (collector.output.required) {
+ checkbox.required = true;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const checkbox = document.createElement('input'); | |
| checkbox.type = 'checkbox'; | |
| checkbox.id = collector.output.key; | |
| checkbox.name = collector.output.key || 'single-checkbox-field'; | |
| checkbox.checked = collector.output.value as boolean; | |
| checkbox.value = 'checked'; | |
| const checkbox = document.createElement('input'); | |
| checkbox.type = 'checkbox'; | |
| checkbox.id = collector.output.key; | |
| checkbox.name = collector.output.key || 'single-checkbox-field'; | |
| checkbox.checked = collector.output.value as boolean; | |
| checkbox.value = 'checked'; | |
| if (collector.output.required) { | |
| checkbox.required = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/davinci-app/components/boolean.ts` around lines 34 - 39, The
SingleCheckboxField's required flag isn't applied to the created checkbox
element; update the element creation in the component that builds the checkbox
(where checkbox is created from collector.output) to set checkbox.required =
!!collector.output.required (and optionally
checkbox.setAttribute('aria-required','true') for accessibility) so the HTML
required validation is honored; ensure you reference collector.output.required
and the checkbox element in the SingleCheckboxField handling logic.
Comment on lines
+46
to
+49
| checkbox.addEventListener('change', (event) => { | ||
| const target = event.target as HTMLInputElement; | ||
| updater(target.value === 'checked'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify checkbox event handling in similar components
# Search for other checkbox change event handlers in the codebase
rg -n -A3 -B3 "addEventListener\s*\(\s*['\"]change['\"]" --glob "*.ts" --glob "*.js" -g '!node_modules'Repository: ForgeRock/ping-javascript-sdk
Length of output: 7149
🏁 Script executed:
cat -n e2e/davinci-app/components/boolean.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 2558
Use target.checked instead of target.value === 'checked'.
The checkbox value attribute (line 39) is a static string that never changes based on checkbox state. The expression target.value === 'checked' will always evaluate to true, regardless of whether the checkbox is checked or unchecked. This breaks the boolean field functionality. Use target.checked to get the actual checkbox state, which is also the pattern used in other checkbox handlers throughout the codebase (e.g., terms-and-conditions.ts, confirmation.ts).
Proposed fix
checkbox.addEventListener('change', (event) => {
const target = event.target as HTMLInputElement;
- updater(target.value === 'checked');
+ updater(target.checked);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| checkbox.addEventListener('change', (event) => { | |
| const target = event.target as HTMLInputElement; | |
| updater(target.value === 'checked'); | |
| }); | |
| checkbox.addEventListener('change', (event) => { | |
| const target = event.target as HTMLInputElement; | |
| updater(target.checked); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/davinci-app/components/boolean.ts` around lines 46 - 49, The change
handler for the checkbox uses the static value attribute and compares
target.value === 'checked', which is always true; update the event listener on
checkbox (the callback passed to checkbox.addEventListener('change', ...)) to
call updater with the actual boolean state using target.checked instead of
comparing target.value, i.e., replace the expression target.value === 'checked'
with target.checked so the updater receives the real checked state.
Comment on lines
+196
to
+198
| case 'SINGLE_CHECKBOX': { | ||
| return returnValidatedBooleanCollector(field, idx); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the signature and implementation of returnValidatedBooleanCollector
# Find the implementation of returnValidatedBooleanCollector
ast-grep --pattern $'export function returnValidatedBooleanCollector($$$) {
$$$
}'
# Also search for its usage to understand the expected parameters
rg -n -A10 "returnValidatedBooleanCollector" --type tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 4218
🏁 Script executed:
#!/bin/bash
# Check returnSingleValueCollector implementation
rg -n -B5 -A15 "export function returnSingleValueCollector" packages/davinci-client/src/lib/collector.utils.ts
# Check returnTextCollector signature and implementation
rg -n -B5 -A15 "export function returnTextCollector" packages/davinci-client/src/lib/collector.utils.ts
# Get more context around the SINGLE_CHECKBOX case in node.reducer.ts
# to understand where data comes from
rg -n -B20 "case 'SINGLE_CHECKBOX'" packages/davinci-client/src/lib/node.reducer.ts | head -50Repository: ForgeRock/ping-javascript-sdk
Length of output: 3362
🏁 Script executed:
#!/bin/bash
# Get the full implementation of returnSingleValueCollector
sed -n '155,200p' packages/davinci-client/src/lib/collector.utils.ts
# Also check what happens after line 200 to see how data is used
sed -n '155,250p' packages/davinci-client/src/lib/collector.utils.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 4790
🏁 Script executed:
#!/bin/bash
# Get more context around SINGLE_CHECKBOX to understand the data variable
# and see how similar field types handle boolean-like data
sed -n '150,210p' packages/davinci-client/src/lib/node.reducer.ts
# Also check RADIO and DROPDOWN cases mentioned in the review
rg -n -B2 -A5 "case 'RADIO':|case 'DROPDOWN':" packages/davinci-client/src/lib/node.reducer.ts | head -40Repository: ForgeRock/ping-javascript-sdk
Length of output: 2838
🏁 Script executed:
#!/bin/bash
# Check how the data value flows through for ValidatedBooleanCollector
# See if there's any conversion from boolean to string
sed -n '210,250p' packages/davinci-client/src/lib/collector.utils.ts
# Also check if there are TypeScript type definitions for ValidatedBooleanCollector
rg -n "ValidatedBooleanCollector" packages/davinci-client/src --type ts | head -20Repository: ForgeRock/ping-javascript-sdk
Length of output: 3503
Add prefill support for SINGLE_CHECKBOX field type.
The SINGLE_CHECKBOX case doesn't extract or pass the data variable to returnValidatedBooleanCollector, unlike all other field types (e.g., TEXT, RADIO, DROPDOWN). This prevents checkboxes from supporting prefilled values from formData. Both the call site in node.reducer.ts and the returnValidatedBooleanCollector function need to be updated to pass the data through to returnSingleValueCollector, similar to how returnTextCollector and returnSingleSelectCollector handle it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/davinci-client/src/lib/node.reducer.ts` around lines 196 - 198, The
SINGLE_CHECKBOX branch in node.reducer.ts currently calls
returnValidatedBooleanCollector(field, idx) without passing the prefill value;
update the case for 'SINGLE_CHECKBOX' to extract the data variable from formData
(same way other branches do) and call returnValidatedBooleanCollector(field,
idx, data). Then update returnValidatedBooleanCollector to accept the extra data
parameter and forward it into returnSingleValueCollector so SINGLE_CHECKBOX
supports prefilled values, mirroring how returnTextCollector and
returnSingleSelectCollector handle the data.
ancheetah
force-pushed
the
SDKS-4926-single-checkbox
branch
from
f4d8163 to
af9e242
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/davinci-app/main.ts (1)
298-300: ⚡ Quick winConsider adding validation support for ValidatedBooleanCollector.
The
ValidatedBooleanCollectorhandler passes onlydavinciClient.update(collector)tobooleanComponent, but the collector type includes validation rules and the name indicates validation support. Compare withTextCollectorhandling (lines 234-240), which passes bothupdate()andvalidate().If the boolean component needs to enforce "required" validation (e.g., mandatory checkbox acceptance), consider passing
davinciClient.validate(collector)as a third argument.♻️ Potential enhancement
} else if (collector.type === 'ValidatedBooleanCollector') { - booleanComponent(formEl, collector, davinciClient.update(collector)); + booleanComponent( + formEl, + collector, + davinciClient.update(collector), + davinciClient.validate(collector), + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/davinci-app/main.ts` around lines 298 - 300, The ValidatedBooleanCollector branch currently calls booleanComponent(formEl, collector, davinciClient.update(collector)) but omits validation; modify the ValidatedBooleanCollector handling so booleanComponent receives the validate handler as well (e.g., pass davinciClient.validate(collector) in addition to davinciClient.update(collector)), mirroring how TextCollector supplies both update() and validate(); update the call site at the ValidatedBooleanCollector branch to include davinciClient.validate(collector) so required/other validation rules on the collector are enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@e2e/davinci-app/main.ts`:
- Around line 298-300: The ValidatedBooleanCollector branch currently calls
booleanComponent(formEl, collector, davinciClient.update(collector)) but omits
validation; modify the ValidatedBooleanCollector handling so booleanComponent
receives the validate handler as well (e.g., pass
davinciClient.validate(collector) in addition to
davinciClient.update(collector)), mirroring how TextCollector supplies both
update() and validate(); update the call site at the ValidatedBooleanCollector
branch to include davinciClient.validate(collector) so required/other validation
rules on the collector are enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ff5f004-90c6-4033-83d2-2d5eaa35204e
📒 Files selected for processing (15)
.gitignoree2e/davinci-app/components/boolean.tse2e/davinci-app/main.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/client.types.tspackages/davinci-client/src/lib/collector.types.test-d.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.types.test-d.tspackages/davinci-client/src/lib/node.types.tspackages/davinci-client/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/davinci-client/tsconfig.json
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- packages/davinci-client/src/lib/node.types.ts
- packages/davinci-client/src/lib/node.types.test-d.ts
- packages/davinci-client/api-report/davinci-client.api.md
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/davinci-client/src/lib/client.types.ts
- e2e/davinci-app/components/boolean.ts
- packages/davinci-client/src/lib/client.store.ts
- packages/davinci-client/src/lib/node.reducer.ts
- packages/davinci-client/src/lib/davinci.types.ts
- packages/davinci-client/src/lib/collector.utils.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4926
Description
Adds support for single checkbox component through a ValidatedBooleanCollector
Summary by CodeRabbit